-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP]: Using builder pattern for increased readability in the dsl. #61
base: master
Are you sure you want to change the base?
Conversation
This is interesting. For those playing along at home, this wouldn't replace the current syntax, but would instead add to it. I think my favorite part is the event-handling aspect: button 'Click me' do |button|
button.onclick { |event| puts 'clicked' }
end Because this isn't great: button({
onclick: ->(event) { puts 'clicked' },
}, 'Click me') This gets especially bad when you're building a video player, for example. Depending on how powerful you want your video({
ref: @container,
onclick: method(:toggle),
oncanplay: method(:on_can_play),
onpause: method(:on_pause),
onplay: method(:on_play),
ontimeupdate: method(:on_time_update),
onloadstart: method(:on_load_start),
onloadeddata: method(:on_loaded_data),
onloadedmetadata: method(:on_loaded_metadata),
onload: method(:on_load),
onseeking: method(:on_seeking),
onseeked: method(:on_seeked),
onwaiting: method(:on_waiting),
onstalled: method(:on_stalled),
onvolumechange: method(:on_volume_change),
}, [
source(src: webm_source),
source(src: mp4_source),
]) Realistically, most of those would simply be re-rendering the app because a video({ ref: @container }, [source(src: webm), source(src: mp4)]) do |video|
video.onclick { toggle }
video.oncanplay { on_can_play }
video.onplay { on_play }
video.onpause { on_pause }
# ...
end I think this goes along with the spirit of what blocks are for (passing functionality rather than values) much more than the idea of putting child elements into them. Food for thought: I don't know what the performance implications are. I'm usually hesitant to add much functionality to the element DSL methods primarily because they can get called thousands of times in a single render — the first render is a bit more sensitive to this because the methods have not been optimized until partway through it. I'm not sure what the performance difference is between |
More food for thought: Block handling has a bug in the released versions of Opal (I think there's a fix but it's not backported to a release yet) in examples like what we might be invoking here. For example, instantiating a store = GrandCentral::Store.new(AppState.new(attrs)) do |state, action|
# ...
end In the above example, the block gets assigned to the call to div([div('something')]) do |dubious_div|
# Which div does this get run as?
end It'll work fine if the inner element isn't the same as the outer element (since it will use a different method), but if both calls go to |
Very good feedback. I did not know about the block bug. Agree on the event-handling. And it was completely unintentional ;-) I haven't looked at the generated code yet but I'm sure there is room for improvement. We don't actually need a ruby hash to back the builder, instead we could just set the values directly on the js-object that will get passed to virtual-dom. Perhaps bypass the sanitize-part as well? What's your take on using predefined attributes vs using Perhaps provide an escape hatch like Are there anything special that needs to be done for server-side rendering? Pure-ruby version on the server-side path and an js-optimized in the opal path? |
I'll put this up here for discussion.
#22 has some good background for the design of the dsl.
What I propose is the ability to build nodes using a builder.
Motivating example:
The builder has domain knowledge about what html attributes and events are available
so typo's can be caught without inspecting the dom. If you miss a name it's easy to extend
in client code.
This PR adds the following to the DSL.
Of course,
button.onclick = -> { |event| }
works as wellInitially I considered using the return value from the block as 'content' but that has to many
edge cases. Using << on the builder is much cleaner imho.
Also there is no magic going on here. Ive looked at a few different types of dsl and some
use method_missing to hide the builder object but this is very hard to get right for every
edge case.
This syntax would be opt-in so no changes are needed to old code.
The code attached is functional but needs more work.
So, suggestsions, yays, nays etc